Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not use linux32 when unnecessary #1599

Merged
merged 7 commits into from
Sep 19, 2023

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Sep 6, 2023

The linux32 command is always used for linux i686.
This makes cross building from macOS arm64 impossible at the moment (might depend on docker configuration).

This PR proposes to check the architecture seen inside the container before using (or not) the linux32 command.

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Is this a fix for the '-mnative' error on macOS?

cibuildwheel/oci_container.py Outdated Show resolved Hide resolved
container_machine = call(
self.engine.name, "run", "--rm", self.image, "uname", "-m", capture_stdout=True
).strip()
if container_machine not in {"x86_64", "aarch64"}:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i686 is the only 32-bit arch so far, I think. Perhaps it would be easier to match that directly?

cibuildwheel/oci_container.py Outdated Show resolved Hide resolved
@mayeut
Copy link
Member Author

mayeut commented Sep 7, 2023

Is this a fix for the '-mnative' error on macOS?

No, it's a fix for a deadlock following a call to linux32:

WARNING: The requested image's platform (linux/386) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
3fca67068b8d0aa05202598ccb3d9e80a27327038b85ede10fb6ab3f73b7543e
    + /bin/true
linux32: Failed to set personality to linux32: Invalid argument

I think that the ENTRYPOINT isn't used when you specify a program via run or create?

It's definitely used on run, the program specified via run is passed to the entrypoint. I'll check it's passed via create also and add a test for this.

Turning this into draft until I get time to address all comments.

@mayeut mayeut marked this pull request as draft September 7, 2023 07:58
@joerick
Copy link
Contributor

joerick commented Sep 7, 2023

It's definitely used on run, the program specified via run is passed to the entrypoint. I'll check it's passed via create also and add a test for this.

Ah, perhaps I am confused about the difference between CMD and ENTRYPOINT.

@mayeut
Copy link
Member Author

mayeut commented Sep 7, 2023

I checked https://docs.docker.com/engine/reference/commandline/create/ and since the ENTRYPOINT can be overridden (same as with run), it's used.
So the confusion might indeed come from ENTRYPOINT vs CMD (which is indeed overriden when specifying a custom command).

@mayeut
Copy link
Member Author

mayeut commented Sep 8, 2023

Reaching disk space limits once again. I'll add different commits to free some space.

@mayeut mayeut force-pushed the manylinux-entrypoint branch 2 times, most recently from cfd8996 to 991de88 Compare September 8, 2023 09:28
@mayeut mayeut marked this pull request as ready for review September 8, 2023 11:01
@mayeut
Copy link
Member Author

mayeut commented Sep 8, 2023

seems cirrus-ci windows x86 is having troubles, found cirruslabs/cirrus-ci-docs#1232

edit: this is now fixed.

@mayeut mayeut requested a review from joerick September 14, 2023 06:30
Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

cibuildwheel/oci_container.py Outdated Show resolved Hide resolved
@joerick joerick merged commit 099d397 into pypa:main Sep 19, 2023
18 checks passed
@mayeut mayeut deleted the manylinux-entrypoint branch September 19, 2023 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants